Check flavour to prevent mixed architectures #9126
Conversation
|
This seems to overlap significantly with #9117 including duplicating constants in multiple locations. It feels overkill to not allow e.g. I'm not sure it's necessary to verify the architecture outside of Where I can see a potential for extension, if we really need it, would be having
It would be good to debug and attempt to resolve these. |
Out of a total of six points which make up #9117, this PR improves on the following two:
More thorough and more general checks would be helpful for several reasons, which I explained in the Rationale section above.
Not sure what you mean. This PR doesn’t allow
I don’t see how this statement fits together with your statement a few lines above, i. e. that you’d rather not allow
In our Slack discussions about handling the microarchitecture, I’ve learned from you personally how important it is to have a homogenous Homebrew installation, and how neither a prefix, a Cellar, nor binary Ruby gem extensions should be mixed-and-matched. Learning that, I changed my mind, admitted in our channel that you were right, dismissed all of my work done on my PR-in-progress, and later went on to design and implement it to match your preferred way (as I’ve perceived it). I feel a little surprised to learn it’s somehow less important now.
If we can prevent an obvious error beyond the shadow of a doubt, why would we rather diagnose and fix it after the fact? |
@MikeMcQuaid This is a good and valid point, and I’ll be happy to address it once I completely understand all of what #9117 does. Can you help me understand (grammatically) what the following messages are trying to say?
Edit:
|
@claui Sorry, I edited that ASAP but I guess you missed it (from
This is something else we could avoid being mismatched through e.g.
I was not aware you had a PR in progress, sorry. Similarly, I'm sorry that my communication of preferences were not done better.
I don't think we need to prevent the usage of Homebrew entirely and block it from even running (besides the cases where it's in a known-wrong prefix) under the incorrect architecture.
We don't want to allow installation/reinstallation/upgrade of any formulae in the "wrong" default prefix for the architecture.
It is run before
This. To hopefully front-load some of this I'll discuss some more of the PR body:
I'd like to avoid user configuration here until we feel it's needed from user issues/requests/confusion.
I see the existing
I'm game to have any existing messaging changed.
I believe this is handled sufficiently by the existing code. Additional extensions should be in
Logic could be altered in
I think we should allow all commands except
I agree with not detecting it at all. We could, if necessary, do so when we're already going to display an error to display a better one. |
|
@MikeMcQuaid Did a complete rewrite, trying to incorporate most of the feedback you’ve given, in the hope that it’s going to meet the requirements better. |
**What it does** - Prevent mixing-and-matching architectures in a single Homebrew installation by asserting the current architecture to be equal to the saved Homebrew flavour. - Give clear and useful guidance when called with the wrong architecture. **Rationale** - We must keep users from mixing-and-matching architectures in a single Homebrew installation, be it deliberately or accidentally, even if the user has chosen a non-standard installation location. - While Homebrew already compares prefixes at formula install time, the check at hand is more general and works with non-standard install locations. **How it’s implemented** - The flavour of the Homebrew installation is stored in the Git config variable `homebrew.flavour`. - Possible values of `homebrew.flavour` are `arm64`, `x86_64` and not set. - `homebrew.flavour` being not set means that Homebrew either couldn’t determine the flavour yet or that it isn’t running under macOS. **Compatibility** - Work with non-standard install locations. This is implemented through picking the current architecture on the initial `brew update` invocation, then persisting it as a Git config value on the Homebrew repository. - Work with our alternative installation method, where the directory tree is typically not a Git repository at install time. - Works as expected when running on OSes other than macOS. - Works on older macOS versions, which do not have the `CFBundleIsArchitectureLoadable` yet. - Works as expected when running on Intel-only Macs. - Only check formula installation, uninstallation and upgrading. **Performance considerations** - Do not try to detect Rosetta 2 unless absolutely necessary (i.e. only while preparing an error message).
| HOMEBREW_DEFAULT_PREFIX="/usr/local" | ||
| HOMEBREW_DEFAULT_TEMP="/private/tmp" | ||
| HOMEBREW_MACOS_ARM_DEFAULT_PREFIX="/opt/homebrew" |
There was a problem hiding this comment.
If these stay around given other comments after this:
- they need to be used in
global.rbso that their definitions aren't duplicated - a definition should be present for Linux
| if [[ -n "${HOMEBREW_MACOS_VERSION}" ]] && | ||
| [[ -z "$(git config --get homebrew.flavour 2>/dev/null)" ]] | ||
| then | ||
| if [[ "${HOMEBREW_PREFIX}" = "${HOMEBREW_MACOS_ARM_DEFAULT_PREFIX}" ]]; then | ||
| git config --replace-all homebrew.flavour "arm64" 2>/dev/null | ||
| elif [[ "${HOMEBREW_PREFIX}" = "${HOMEBREW_DEFAULT_PREFIX}" ]]; then | ||
| git config --replace-all homebrew.flavour "x86_64" 2>/dev/null | ||
| else | ||
| git config --replace-all homebrew.flavour "$(uname -m)" 2>/dev/null | ||
| fi | ||
| fi |
There was a problem hiding this comment.
- feels like this should be in
brew.sh(or eveninstall.rb) rather thanupdate.sh - would be good to set a variable inside the
if/elifto DRY up the repeatedgit config --replace-all homebrew.flavour - feels like this should be inferred based on previously/newly installed formulae rather than the current architecture on existing installs (to avoid a single Rosetta/non-Rosetta run setting these incorrectly)
| return | ||
| end | ||
|
|
||
| error_title = "This Homebrew installation in #{HOMEBREW_PREFIX} only works on #{homebrew_flavour} processors." |
There was a problem hiding this comment.
| error_title = "This Homebrew installation in #{HOMEBREW_PREFIX} only works on #{homebrew_flavour} processors." | |
| error_title = "This Homebrew installation in #{HOMEBREW_PREFIX} only works on #{homebrew_flavour} processors!" |
| To use it, you can: | ||
| - run your Terminal from Rosetta 2 or | ||
| - run 'arch -x86_64 brew' instead of 'brew'. |
There was a problem hiding this comment.
| To use it, you can: | |
| - run your Terminal from Rosetta 2 or | |
| - run 'arch -x86_64 brew' instead of 'brew'. | |
| To use it, you can choose between: | |
| - running your Terminal from Rosetta 2 | |
| - running 'arch -x86_64 brew' instead of 'brew'. |
the inline or felt weird
| brew bundle dump | ||
| def check_flavour_matches_architecture | ||
| homebrew_flavour = HOMEBREW_REPOSITORY.cd do | ||
| Utils.popen_read("git", "config", "--get", "homebrew.flavour").chomp.presence |
There was a problem hiding this comment.
Could consider passing this as a global variable, too, so it only needs read once.
There was a problem hiding this comment.
I did that in my first draft, which:
[introduced] a read-only HOMEBREW_FLAVOUR environment variable that tells us under which architecture a given Homebrew installation can be run.
I removed it after your comment that said:
I'd like to avoid user configuration here until we feel it's needed from user issues/requests/confusion.
That environment variable wasn’t user configuration. It was read-only, would be set by Homebrew, ignoring whatever its initial value is. Its purpose was to pass it as a global variable so it only needs to be read once. I removed it anyway according to your request.
So you’d prefer me to add that variable back, right?
There was a problem hiding this comment.
That environment variable wasn’t user configuration. It was read-only, would be set by Homebrew, ignoring whatever its initial value is. Its purpose was to pass it as a global variable so it only needs to be read once. I removed it anyway according to your request.
I didn't request you remove it, I requested we avoid it being user configurable as it wasn't clear from the proposal whether it would be or not.
So you’d prefer me to add that variable back, right?
Yes, I think it would be worth considering if it aligns with the fix for #9126 (comment)
#9126 (comment) is another global variable that should be passed from Bash into Ruby to avoid duplication if it's needed there.
There was a problem hiding this comment.
@MikeMcQuaid Ok, thanks. Will look for the old commit, restore it and apply your suggestions.
|
Bike shedding here, but I'd prefer to name it |
|
|
Line 6 in 86631b4
|
Being even more pedantic, though: the processor isn't changing, is it? |
I think I follow your meaning, that the physical processor is not somehow physically morphing when you run Rosetta. Nonetheless |
|
@claui Any news here? Thanks! |
1 similar comment
|
@claui Any news here? Thanks! |
@sjackman @MikeMcQuaid Will rename |
I think it would be good to use the same name for both and, if needed, use |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
Fyi, meant to work on this but still struggling with my recent Big Sur update. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
Review period ended. |
|
Going to try and rebase this weekend. |
|
@claui That'd be great. If you do so: can you also address (or comment on why your're not addressing) any pending comments here and ensure CI is green? Thanks! |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
Passing on this for now to avoid stalebot ping-pong but will happily rereview a new version 👍🏻 |
allmost code paths that came to mind,bothat least on Apple Silicon (with and without Rosetta) for nowand on an Intel-only Mac.brew stylewith your changes locally?brew testswith your changes locally? Massive amount of errors on my Intel Mac but all of them due to flaky tests.brew manlocally and committed any changes?Update: Rewritten to accommodate the feedback.
What it does
Introduce a read-onlyHOMEBREW_FLAVOURenvironment variable that tells us under which architecture a given Homebrew installation can be run.Prevent mixing-and-matching architectures in a single Homebrew installation by asserting the current architecture to be equal to
HOMEBREW_FLAVOURGive clear and useful guidance when called with the wrong architecture.
DisplayHOMEBREW_FLAVOURinbrew configfor diagnostic purposes if it’s set.Rationale
We must keep users from mixing-and-matching architectures in a single Homebrew installation, be it deliberately or accidentally, even if the user has chosen a non-standard installation location.
While Homebrew already compares prefixes at formula install time, the check at hand is more general and works with non-standard install locations.
It’s important to prevent mixed binary code in native Ruby gems. Therefore we need to check not only formula installations but all Homebrew commands that may install or update a Ruby gem.How it’s implemented
The flavour of the Homebrew installation is stored in the Git config variable
homebrew.flavour.Possible values of
homebrew.flavourarearm64,x86_64and not set.homebrew.flavourbeing not set means that Homebrew either couldn’t determine the flavour yet or that it isn’t running under macOS.Expose bothHOMEBREW_MACOS_ARCHITECTUREandHOMEBREW_FLAVOURto Homebrew. This is soupgrade.shcan figure out and pin a value if it’s not installed in a standard location, and sobrew configcan display it for diagnostic purposes.Compatibility
Work with non-standard install locations. This is implemented through picking the current architecture on the initial
brew updateinvocation, then persisting it as a Git config value on the Homebrew repository.Work with our alternative installation method, where the directory tree is typically not a Git repository at install time.
Works as expected when running on OSes other than macOS.
Works on older macOS versions, which do not have the
CFBundleIsArchitectureLoadableyet.Works as expected when running on Intel-only Macs
(but still provides HOMEBREW_FLAVOUR in that case).Always allow theOnly check formula installation, uninstallation and upgrading.--(dash-dash) commands,brew configandbrew shellcheck.Performance considerations
Do not callgit configunless possible and absolutely necessary.Do not try to detect Rosetta 2 unless absolutely necessary (i.e.
only while preparing an error message).